-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[samsungtv] Frame TV Fixes, Improvements and New Channels #11895
Conversation
3f66888
to
5260859
Compare
Will do, any suggestions on a more systematic way of detecting? |
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing a mooit so quick. While looking at the changes that have been made, i found some new. I also noticed that not all comments are addressed, possibly because they are behind the 'Load more' button. (exactly those are missing)
Some comments from this and the other review round can also be applied to different areas of this PR, would be nice if you can try to apply those suggestions on other places too.
....openhab.binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WolSend.java
Outdated
Show resolved
Hide resolved
....openhab.binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WolSend.java
Outdated
Show resolved
Hide resolved
....openhab.binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WolSend.java
Show resolved
Hide resolved
....openhab.binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WolSend.java
Show resolved
Hide resolved
....openhab.binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WolSend.java
Show resolved
Hide resolved
...binding.samsungtv/src/main/java/org/openhab/binding/samsungtv/internal/WakeOnLanUtility.java
Outdated
Show resolved
Hide resolved
...ngtv/src/main/java/org/openhab/binding/samsungtv/internal/service/SmartThingsApiService.java
Outdated
Show resolved
Hide resolved
...ngtv/src/main/java/org/openhab/binding/samsungtv/internal/service/SmartThingsApiService.java
Show resolved
Hide resolved
...ngtv/src/main/java/org/openhab/binding/samsungtv/internal/service/SmartThingsApiService.java
Outdated
Show resolved
Hide resolved
...ngtv/src/main/java/org/openhab/binding/samsungtv/internal/service/SmartThingsApiService.java
Outdated
Show resolved
Hide resolved
Yes, missed those. Will try to fix them now. |
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to come back to the readme, but here are some first comments. Very nice to see all the detailed documentation, really usefull for users. We only need to restructure it a bit and present it in a way that it is an easy read and in line with other bindings.
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
There will probably allways be some area's to improve, especially with these many changes, tests and PR age. I like to move forward and merge this. Regressions, new improvements can (and should) have a smaller scope and will certainly be merged faster :-) |
i think I have addressed all comments now. |
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! LGTM. Let’s get some mileage on this with the snapshots. We have some time left before this gets into stable.
@NickWaterton As you have gained deep knowledge of this binding, you might want to create a follow up PR to add yourself to the codeowners file for this binding. |
Thanks for merging this! How do I add myself to the codeowners file, and do a follow up PR? |
Just create a new PR like any other and only edit the codeowners file: |
A PR against the Main branch? or a new branch? |
Just like how you created this PR. Create a feature branch agains main, chagne the file, publish the branch and push the changes to your github, create a pr here. |
OK, PR #16784 created. Also @jlaur found a bug in the legacy code. (#16783) I have a fix for it. Do I do the same, branch, fix, push, PR etc to fix it? Thanks. |
This PR is to update the samsungtv binding to better support the Frame TV.
Fixes: #11285
Fixes: #11284
It adds 5 new channels specifically for Frame TV's.
These new channels allow the art brightness and colour to be adjusted, plus the current artwork is given and can be selected. A thumbnail image is downloaded (of the current art), and a string interface to the art websocket is provided (so that additional commands can be sent).
In addition it addresses the problem that >2020 TV's cannot launch apps because of an API being removed by Samsung. A work around for the problem has been implemented, restoring this functionality.
This restores the function of the sourceApp and url channels.
A new feature has also been added to allow connection to the Smartthings API, which restores the function of 4 channels which otherwise would not function on TV's >2016 (sourceName, sourceId, channel and channelName).
The keyCode channel has been upgraded to allow:
KeyCode timing has been changed to use the scheduler, in place of fixed delays, to make the binding more responsive.
The sourceApp channel has been changed to allow the setting of slideshows on frame TV's. This channel now reports if art is being displayed or a slideshow (plus duration) in ArtMode, or the current app in TV mode. Blank if there is no current App displayed in TV mode (or the TV is off).
The original functions continue to work as normal on non Frame TV's - there are no breaking changes to the interface.
The documentation has been updated to reflect these changes, and explain in more detail how the binding can be used with examples.
The binding has been tested on TV's ranging from 2012 to 2021, but only 7 models have been tested, so more testing would be useful, hence the [WIP] label.
2014 and 2015 (Models H and J) have encrypted remotecontrol websockets, so these models are still not supported.
Signed-off-by: Nick Waterton n.waterton@outlook.com
org.openhab.binding.samsungtv-4.2.0-SNAPSHOT.jar
org.openhab.binding.samsungtv-4.2.0-SNAPSHOT.kar